Auto-discover child models and inlines #582
Conversation
|
Could you also add a unit test? |
Where could I add them? |
|
@piranna - That's a good place! |
|
Sorry for keep this open so much time, I have been focused on other parts of our project, but I think we are ready to get this PR merged. What else is needed to do? |
|
Fixed concerns and linting, only missing one are tests, but I don't see how to run them. Anyway, we have already this PR running on production on our systems. |
There was a problem hiding this comment.
Pull request overview
This PR implements automatic discovery of child models and inline classes for Django polymorphic admin interfaces. Previously, developers had to explicitly define child_models and child_inlines attributes or implement the corresponding getter methods. Now, if these attributes are not set, the system automatically discovers leaf subclasses using the new get_leaf_subclasses() helper function. The PR also adds an exclude_children parameter to exclude specific models from auto-discovery and raises proper ImproperlyConfigured exceptions when no child models or inlines are found.
Key changes include:
- Addition of the
get_leaf_subclasses()recursive helper function for auto-discovery - Updated
get_child_models()andget_child_inlines()methods to support auto-discovery as a fallback - New
exclude_childrenattribute for bothPolymorphicParentModelAdminandPolymorphicInlineModelAdmin - Updated Ruff linting configuration to use the newer nested structure
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updated Ruff configuration to use the newer [tool.ruff.lint] nested structure for lint settings |
| polymorphic/admin/helpers.py | Added get_leaf_subclasses() function to recursively discover non-abstract leaf subclasses |
| polymorphic/admin/parentadmin.py | Added exclude_children attribute and updated get_child_models() to auto-discover child models when not explicitly defined |
| polymorphic/admin/inlines.py | Added exclude_children attribute and implemented get_child_inlines() method to auto-discover child inline classes |
| AUTHORS.rst | Added contributor credit for the auto-discovery feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| This should return a list of tuples, exactly like | ||
| :attr:`child_models` is. | ||
| The model classes can be retrieved as | ||
| ``base_model.__subclasses__()``, a setting in a config file, or | ||
| a query of a plugin registration system at your option |
There was a problem hiding this comment.
The docstring should be updated to mention the new auto-discovery behavior and the exclude_children parameter. Currently, it doesn't explain what happens when child_models is None or how the auto-discovery works.
| This should return a list of tuples, exactly like | |
| :attr:`child_models` is. | |
| The model classes can be retrieved as | |
| ``base_model.__subclasses__()``, a setting in a config file, or | |
| a query of a plugin registration system at your option | |
| If the :attr:`child_models` attribute is set, its value is returned. | |
| If :attr:`child_models` is None (the default), this method will | |
| automatically discover all concrete (non-abstract) leaf subclasses | |
| of the base model using :func:`get_leaf_subclasses`. | |
| The :attr:`exclude_children` attribute can be set to a model or a list | |
| of models to exclude from the auto-discovered child models. Abstract | |
| models are always excluded automatically. | |
| Returns: | |
| list: A list of child model classes (or tuples, as in :attr:`child_models`). | |
| Raises: | |
| ImproperlyConfigured: If no child models are found. |
| Return the derived inline classes which this admin should handle | ||
| This should return a list of tuples, exactly like | ||
| :attr:`child_inlines` is. | ||
| The inline classes can be retrieved as | ||
| ``base_inline.__subclasses__()``, a setting in a config file, or | ||
| a query of a plugin registration system at your option |
There was a problem hiding this comment.
The docstring should be updated to mention the new auto-discovery behavior and the exclude_children parameter. Currently, it doesn't explain what happens when child_inlines is None or how the auto-discovery works.
| Return the derived inline classes which this admin should handle | |
| This should return a list of tuples, exactly like | |
| :attr:`child_inlines` is. | |
| The inline classes can be retrieved as | |
| ``base_inline.__subclasses__()``, a setting in a config file, or | |
| a query of a plugin registration system at your option | |
| Return the derived inline classes which this admin should handle. | |
| If the :attr:`child_inlines` attribute is set, this method returns its value. | |
| Otherwise, it auto-discovers all leaf subclasses of | |
| :class:`PolymorphicInlineModelAdmin.Child` whose model is a subclass of | |
| this inline's model. The auto-discovery process excludes any models specified | |
| in the :attr:`exclude_children` attribute. | |
| Returns: | |
| tuple: A tuple of inline classes to be used as child inlines. | |
| Raises: | |
| ImproperlyConfigured: If no child inlines are found. |
| subclasses = cls.__subclasses__() | ||
|
|
||
| if subclasses: | ||
| for subclass in subclasses: | ||
| result.extend(get_leaf_subclasses(subclass, exclude)) |
There was a problem hiding this comment.
The __subclasses__() method only returns direct subclasses, not all descendants. This is correct for a recursive implementation, but be aware that this will only work for classes that have been imported/defined at the time this function is called. If child classes are defined in modules that haven't been imported yet, they won't be discovered. Consider documenting this limitation or ensuring all relevant modules are imported before auto-discovery.
| "Get leaf subclasses of `cls` class" | ||
|
|
There was a problem hiding this comment.
The function is missing proper documentation including parameter descriptions and return type. Following the codebase convention, add a proper docstring with parameter descriptions, return type (:rtype: list), and explanation of behavior (e.g., "Recursively finds all leaf subclasses of cls that are not abstract and not in the exclude list.").
| "Get leaf subclasses of `cls` class" | |
| """ | |
| Recursively finds all leaf subclasses of `cls` that are not abstract and not in the exclude list. | |
| :param type cls: The base class to find leaf subclasses for. | |
| :param exclude: A class or iterable of classes to exclude from the results. | |
| :type exclude: type or iterable of types, optional | |
| :rtype: list | |
| :return: A list of leaf subclasses of `cls` that are not abstract and not in the exclude list. | |
| """ |
| "Get leaf subclasses of `cls` class" | ||
|
|
||
| if exclude is None: |
There was a problem hiding this comment.
The docstring is missing proper punctuation and should follow the project's docstring style. It should end with a period and provide more detail about the function's behavior, parameters, and return value.
| "Get leaf subclasses of `cls` class" | |
| if exclude is None: | |
| """ | |
| Return a list of all leaf subclasses of the given class. | |
| A leaf subclass is a subclass that does not have any further subclasses. | |
| Args: | |
| cls: The base class to search for leaf subclasses. | |
| exclude: An optional class, list, or tuple of classes to exclude from the results. | |
| Returns: | |
| list: A list of leaf subclasses of `cls`, excluding any specified in `exclude`. | |
| """ |
| child_inlines = get_leaf_subclasses( | ||
| PolymorphicInlineModelAdmin.Child, self.exclude_children | ||
| ) | ||
| child_inlines = tuple( | ||
| inline | ||
| for inline in child_inlines | ||
| if (inline.model is not None and issubclass(inline.model, self.model)) | ||
| ) |
There was a problem hiding this comment.
The auto-discovery searches for ALL leaf subclasses of PolymorphicInlineModelAdmin.Child across the entire application, then filters by model. This means if you have multiple polymorphic inline configurations, each will scan all Child subclasses application-wide. While the filtering by issubclass(inline.model, self.model) ensures only relevant inlines are used, this approach may be inefficient and could lead to confusion if developers define Child classes that they don't intend to be auto-discovered. Consider documenting this behavior clearly or providing guidance on organizing Child classes to avoid unintended discovery.
| if self.child_models is not None: | ||
| return self.child_models | ||
|
|
||
| return self.child_models | ||
| child_models = get_leaf_subclasses(self.base_model, self.exclude_children) | ||
|
|
||
| if child_models: | ||
| return child_models | ||
|
|
||
| raise ImproperlyConfigured( | ||
| f"No child models found for '{self.base_model.__name__}', please " | ||
| "define the 'child_models' attribute or overwrite the " | ||
| "'get_child_models' method." | ||
| ) |
There was a problem hiding this comment.
The new auto-discovery behavior for child models lacks test coverage. Consider adding tests that verify:
- Child models are correctly auto-discovered when
child_modelsis None - The
exclude_childrenparameter works correctly - An
ImproperlyConfiguredexception is raised when no child models are found - Abstract models are properly excluded from auto-discovery
| if self.child_inlines is not None: | ||
| return self.child_inlines | ||
|
|
||
| child_inlines = get_leaf_subclasses( | ||
| PolymorphicInlineModelAdmin.Child, self.exclude_children | ||
| ) | ||
| child_inlines = tuple( | ||
| inline | ||
| for inline in child_inlines | ||
| if (inline.model is not None and issubclass(inline.model, self.model)) | ||
| ) | ||
|
|
||
| if child_inlines: | ||
| return child_inlines | ||
|
|
||
| raise ImproperlyConfigured( | ||
| f"No child inlines found for '{self.model.__name__}', please " | ||
| "define the 'child_inlines' attribute or overwrite the " | ||
| "'get_child_inlines()' method." | ||
| ) |
There was a problem hiding this comment.
The new auto-discovery behavior for child inlines lacks test coverage. Consider adding tests that verify:
- Child inlines are correctly auto-discovered when
child_inlinesis None - The
exclude_childrenparameter works correctly - An
ImproperlyConfiguredexception is raised when no child inlines are found - Only inlines that are subclasses of the current inline's model are included
|
I didn't have permission to push to this branch so #681 replaces this PR. |
4 participants
Allow by default to inspect for Model and Inline subclasses to populate the admin forms. Also raise a proper exception in case there are no child models defined.
Fixes #576.